-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Security team: AWP] Session view: Alert details tab #127500
Conversation
x-pack/plugins/session_view/public/components/detail_panel_alert_actions/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/detail_panel_alert_actions/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/detail_panel_alert_actions/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/detail_panel_alert_group_item/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/detail_panel_alert_tab/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/session_view/hooks.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/session_view/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
x-pack/plugins/session_view/public/components/session_view_detail_panel/index.tsx
Show resolved
Hide resolved
x-pack/plugins/session_view/public/components/session_view_detail_panel/index.tsx
Show resolved
Hide resolved
export const registerAlertsRoute = (router: IRouter) => { | ||
router.get( | ||
{ | ||
path: ALERTS_ROUTE, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing this directly is probably kinda dangerous, and there's some hidden nuance in this index I think, with rbac and building block alerts and things like that. Probably what should be done is make the session_view dependent on the ruleRegistry plugin, and make use of the AlertsClient exported by that plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples of this would be appreciated, not sure how to go about accessing AlertsClient after making rule_registry a dep. I assume it's not as simple as just importing AlertsClient from rule_registry/server/index.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitodrummer you can use the rule_registry
plugin start contract RuleRegistryPluginStartContract
for accessing AlertsClient, by calling the method getRacClientWithRequest
.
I didn't find the code for AlertsClient , but here is the similar usage of RulesClient: you get it by calling getRulesClientWithRequest
, code x-pack/plugins/monitoring/server/plugin.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rule_registry is added as a dep and I'm making use of getRacClientWithRequest in alerts_route.ts. Let me know if the plugin setup looks good.
const alertsQuery = useFetchSessionViewAlerts(sessionEntityId); | ||
const { data: alerts, error: alertsError, isFetching: alertsFetching } = alertsQuery; | ||
|
||
const hasData = alerts && data && data.pages?.[0].events.length > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is alerts
here in case fetching alerts ran into error? Because I imagine if we don't have alerts, it will just be a truthy []
. If it is for errors, would the alertsError be enough for error handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's to avoid passing an undefined to ProcessTree as the prop expects an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alerts is initially undefined until the react-query fetch completes.
x-pack/plugins/rule_registry/common/assets/field_maps/ecs_field_map.ts
Outdated
Show resolved
Hide resolved
); | ||
}); | ||
|
||
describe('doSearch(client, sessionEntityId)', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is great, but it tests the function executed inside and it doesn't test the route itself. Please check the examples here.
As the long term vision I suggest to split the routes and its internal logic to the library files or SessionViewClient
, if you want to expose this logic not only with the routs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Suggesting to add the routes tests, could be a follow up PR and very useful.
💛 Build succeeded, but was flakyTest FailuresMetrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Summary
Issue: #126065
Implementation complete for the Alerts tab in the Session View details panel. I've also broken the loading of alerts and process events apart into their own routes. The new alerts_route will load a fixed number of alerts for the session (currently 1000). This is done so we don't need to coordinate loading pages of process events and alert events in order to show status in process tree.
The alerts tab will show all alerts for the session with the ability to focus on an investigatedAlert (e.g when loading session view from the alerts page). This alert appears sticky at the top of the scroll area with special UX treament.
The alerts tab also supports the ability to group alerts by rule.
The following actions can be taken on each alert in the list:
Checklist
Delete any items that are not applicable to this PR.